Improve checks for scalar _get_bits methods#1845
Improve checks for scalar _get_bits methods#1845peterdettman wants to merge 1 commit intobitcoin-core:masterfrom
Conversation
| VERIFY_CHECK(count > 0 && count <= 32); | ||
| VERIFY_CHECK((offset + count - 1) >> 6 == offset >> 6); | ||
| VERIFY_CHECK(offset <= 256 - count); | ||
| VERIFY_CHECK((offset + count - 1) >> 5 == offset >> 5); |
There was a problem hiding this comment.
I'm convinced this will not break things and I like this more after fiddling with the bits.
But feels like the original check combined with line 43 was already sufficient for checking we are not able to cross limbs when trying to collect bits from the scalar.
One such case could be: offset = 31.
With the original check:
RHS: offset >> 6 = 0
LHS: (31 + count - 1) >> 6 => count < 34 so that RHS == LHS.
But line 43 already guarantee that count <= 32. Similar for the other limbs.
| VERIFY_CHECK(offset + count <= 256); | ||
| VERIFY_CHECK(offset <= 256 - count); |
There was a problem hiding this comment.
I wonder if those aren't the same check.
I mean, offset + count could overflow while 256 - count can't because of the first check.
Is that the intended fix?
| VERIFY_CHECK(offset + count <= 256); | ||
| VERIFY_CHECK(offset <= 256 - count); | ||
|
|
||
| if ((offset + count - 1) >> 6 == offset >> 6) { |
There was a problem hiding this comment.
Here, we are checking if we are not crossing limbs when collecting bits. I think it's ok to inline the function in the following, but not so much if we are testing a different condition compared to the called function, which is what this PR proposes by changing line R45.
Why not
| if ((offset + count - 1) >> 6 == offset >> 6) { | |
| if ((offset + count - 1) >> 5 == offset >> 5) { |
as well?
edilmedeiros
left a comment
edilmedeiros
left a comment
There was a problem hiding this comment.
Overall positive, this PR is indeed improving parameter checking.
E.g. the invalid combination offset = 300; count = 10; would pass the original checks in secp256k1_scalar_get_bits_limb32.
Do you think it's worth adding some unit tests to avoid future regression?
3 participants
Improves the
VERIFY_CHECKs in all_scalar_get_bits_limb32and_scalar_get_bits_varmethods.The initial prompt was noticing that scalar_4x64_impl/
secp256k1_scalar_get_bits_limb32was not restricting to 32-bit limbs correctly. Then missing range checks foroffsetwere added and all such checks rewritten to avoid overflow.With these changes, the _low and _4x64 impls of
_get_bits_varcan no longer forward to_get_bits_limb32, so those calls were inlined instead.